fix: prevent symlink path traversal in file writes#481
Conversation
When .swamp/ subdirectories (e.g. outputs, data, secrets) are replaced with symlinks pointing outside the repository, swamp follows the symlink and writes sensitive data to attacker-controlled locations. The existing assertPathContained() uses resolve() which only normalizes paths lexically — it does not follow symlinks. A symlink directory passes the check even when it points outside .swamp/. Add a shared assertSafePath() utility that uses Deno.realPath() to resolve symlinks before verifying containment. Integrate it at every write location across the codebase. Critically, the boundary for each check is the .swamp/ directory (or repo root for the index service), NOT the subdirectory that could itself be the symlink. Using the subdirectory as boundary would make the check a no-op since both sides resolve through the same symlink. Closes #479 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
Approved - This is a well-implemented security fix that correctly addresses the symlink path traversal vulnerability (#479).
What I Verified
✅ No blocking issues found
Code Quality
- TypeScript strict mode compliance - no
anytypes - Named exports used throughout (no default exports)
- AGPLv3 license headers present on all new files
- Code follows project conventions and formatting
Security Implementation
assertSafePath()correctly usesDeno.realPath()to resolve symlinks before checking path containment- The boundary is correctly set at the parent
.swamp/directory level, not the potentially-symlinked subdirectories - this prevents the "naive fix" bypass described in the PR PathTraversalErrorprovides actionable error details (path, boundary, resolved target)resolveRealPath()properly handles non-existent paths by walking up to nearest existing ancestor
Test Coverage
- 9 comprehensive test cases in
safe_path_test.ts(adjacent to source file per convention) - Tests cover: valid paths, boundary equality, symlink escapes, non-existent paths, internal symlinks, boundary-as-symlink attack vector, error details
- Importantly includes the "using symlinked directory as boundary does NOT catch attack" test that documents why the parent boundary is critical
DDD/Architecture
- Infrastructure concern properly isolated in
persistencelayer - No domain model contamination
- Existing repositories call the shared utility consistently
CI Status
- Lint, test, and format checks pass
Suggestions (non-blocking)
-
Path separator portability: Line 112 in
safe_path.tsuses hardcoded"/"inresolvedBoundary + "/". On Windows,Deno.realPath()returns paths with\separators, which could cause false negatives. If Windows support becomes needed, consider using@std/path'sSEPconstant. Not blocking since this appears to be a Unix-focused tool. -
Documentation opportunity: There's an inherent TOCTOU (time-of-check-time-of-use) race between path validation and file operations. This is a common limitation that's difficult to fully prevent, and the current implementation provides significant security improvement. Consider documenting this as a known limitation if not already covered.
Well done on the thorough implementation and excellent PR description explaining the attack scenarios!
🤖 Generated with Claude Code
1 participant
Summary
Fixes #479 — symlink path traversal allowing writes outside the repository.
When
.swamp/subdirectories (e.g.outputs,data,secrets) are replaced with symlinks pointing outside the repository, swamp follows the symlink and writes sensitive data (resolved secrets, computation results) to the attacker-controlled location.The vulnerability
The existing
assertPathContained()usesresolve()which only normalizes paths lexically — it does not follow symlinks. A symlinked directory passes the check even when it points outside.swamp/.The fix
A new shared
assertSafePath()utility usesDeno.realPath()to resolve symlinks before verifying path containment. It's integrated at every write location across the codebase (17 files, 34 call sites).Attack scenario: before vs after
Attack setup:
.swamp/outputsis replaced with a symlink →/tmp/evilBefore the fix — no symlink-aware check exists:
Naive fix (wrong boundary) — using the subdirectory as boundary:
Both sides resolve through the same symlink, making the check a no-op.
Correct fix (parent boundary) — using
.swamp/as boundary:User impact
PathTraversalErroris thrown with the path, boundary, and resolved target in the message.assertPathContained()checks inUnifiedDataRepositoryandYamlVaultConfigRepositoryare kept as defense-in-depth.Plan vs implementation deviations
The original plan specified subdirectory-level boundaries (e.g.
.swamp/outputs,.swamp/data,.swamp/secrets). During implementation review, this was identified as incorrect — using the potentially-symlinked directory as its own boundary makes the check ineffective. All boundaries were raised to the.swamp/directory (or repo root for the index service).swampPath(repoDir, SWAMP_SUBDIRS.xxx)swampPath(repoDir)UnifiedDataRepositoryswampPath(repoDir, SWAMP_SUBDIRS.data)swampPath(repoDir)YamlOutputRepositoryswampPath(repoDir, SWAMP_SUBDIRS.outputs)swampPath(repoDir)LocalEncryptionVaultProviderswampPath(baseDir, SWAMP_SUBDIRS.secrets)swampPath(baseDir)UserModelLoaderswampPath(repoDir, SWAMP_SUBDIRS.bundles)join(repoDir, SWAMP_DATA_DIR)RunFileSinkcallersswampPath(repoDir)SymlinkRepoIndexServicemodelsBaseDir/workflowsBaseDir/vaultsBaseDirthis.repoDirAdditional deviation: the plan didn't mention the
execution_service.tscall site forRunFileSink.register(), which was also protected.Files changed (17)
New:
src/infrastructure/persistence/safe_path.ts—PathTraversalError+assertSafePath()src/infrastructure/persistence/safe_path_test.ts— 9 test casesModified (15):
src/infrastructure/persistence/unified_data_repository.ts— 5 checks (save, append, allocate, symlink)src/infrastructure/persistence/yaml_output_repository.tssrc/infrastructure/persistence/yaml_definition_repository.tssrc/infrastructure/persistence/yaml_evaluated_definition_repository.tssrc/infrastructure/persistence/yaml_workflow_repository.tssrc/infrastructure/persistence/yaml_evaluated_workflow_repository.tssrc/infrastructure/persistence/yaml_workflow_run_repository.tssrc/infrastructure/persistence/yaml_vault_config_repository.tssrc/infrastructure/persistence/json_telemetry_repository.tssrc/infrastructure/logging/run_file_sink.ts— optionalboundaryparamsrc/cli/commands/model_method_run.ts— passes boundary to sinksrc/domain/workflows/execution_service.ts— passes boundary to sinksrc/domain/vaults/local_encryption_vault_provider.ts— checks in put/ensureDirsrc/domain/models/user_model_loader.ts— checks in bundleWithCachesrc/infrastructure/repo/symlink_repo_index_service.ts— replaced private method with shared utilityTest Plan
deno fmt --check— passesdeno lint— passesdeno check— passesdeno run test— 2079 tests pass (138 steps)deno run compile— binary compiles🤖 Generated with Claude Code